-
-
Notifications
You must be signed in to change notification settings - Fork 18
Add namespace. WPCS Cleanup Part 2 #78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add namespace. WPCS Cleanup Part 2 #78
Conversation
|
I've created a stress test and it seems to work fine. If you have an answer to my question abvove I thing we can merge it. Here is my test case |
|
@mtoensing I'm not seeing the question to respond to. Can you please point me in the right direction? |
mtoensing
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ronalfy Yes, this is an important change. I used the @ syntax because I had problem with try catch in an old php version. Since I see that you generally know what you are doing: is this worth the risk? Or do you say "Relax, there is no risk. This works and it is best practice?
Maybe this was not visible before I set "Request changes" but.I am not sure if we need changes.
| * @return string The modified HTML string with the closing tag(s) added | ||
| */ | ||
| function add_accordion_end( $html, $attributes ) { | ||
| // Check if accordion is enabled either through the function arguments or the options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ronalfy Yes, this is an important change. I used the @ syntax because I had problem with try catch in an old php version. Since I see that you generally know what you are doing: is this worth the risk? Or do you say "Relax, there is no risk. This works and it is best practice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtoensing I can double-check this. Do you recall which version of PHP? Does this plugin have a minimum supported PHP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes minimum is 7.3. see readme
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I've double-checked the readme versions. I tested as far back as WordPress 5.9, as that's the minimum supported version according to the readme.
Here's what I've found.
I tested SimpleTOC 6.9.1 (prior to these changes) and the latest 6.9.4.
From my tests, the minimum working version of the plugin is WordPress 6.6.
The minimum supported PHP version of the plugin is: 7.2.
If these requirements can be updated, then the suppressed error message flag can safely be removed as I'm not receiving any errors with the above setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are the WP supported versions if that helps. WP 6.6 supports a minimum of 7.2, which makes sense.
https://make.wordpress.org/core/handbook/references/php-compatibility-and-wordpress-versions/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.

Ref #76
This PR resumes the WPCS cleanup:
plugin.phpto pass standards.I've tested:
I could use some more help testing the DOM XML Parsing features, as I removed an error suppressor and did a try/catch instead.
Thank you for reviewing!